fix: issue with cerberus only triggering after an edit #433
Conversation
WalkthroughConverted webhook charter processing to non-blocking async calls; added creation-time locking and evault-related unlocking with a delayed (5s) manual sync; simplified charter-change signature deletion; and added a subscriber guard to skip syncing groups that lack a charter. Changes
Sequence Diagram(s)sequenceDiagram
participant Webhook as Webhook (Cerberus)
participant GroupCtrl as GroupController (API)
participant GroupSvc as GroupService (API)
participant Adapter as Adapter
participant Subscriber as Subscriber
alt New Group Creation
Webhook->>+GroupCtrl: POST /group (create)
GroupCtrl->>Adapter: addToLockedIds(group.id)
GroupCtrl->>+GroupSvc: save new group
GroupSvc-->>-GroupCtrl: saved group
GroupCtrl-->>-Webhook: 201 Created
Note right of Webhook: processCharterChange called async\n(with .catch) — no await
end
alt Charter Update / Provision via eVault
Webhook->>+GroupCtrl: PATCH /groups/:id (update charter)
GroupCtrl->>+GroupSvc: updateGroup (merge & save)
GroupSvc->>GroupSvc: detect charterChanged\ndelete signatures if changed
GroupSvc-->>-GroupCtrl: updated group
GroupCtrl->>Adapter: removeFromLockedIds(group.id) (if needsEVault)
GroupCtrl->>GroupCtrl: wait 5s
GroupCtrl->>+GroupSvc: re-fetch full group
GroupCtrl->>Adapter: handleChange("groups", fullGroup) (manual sync)
GroupCtrl-->>-Webhook: 200 OK
Note right of Webhook: processCharterChange called async\n(with .catch) — no await
end
alt Downstream subscriber processing
Webhook->>Subscriber: processCharterChange (async)
Subscriber->>Subscriber: validate data.id
Note over Subscriber: if table == "groups" AND no charter -> skip
Subscriber->>Subscriber: continue processing if charter present
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes
Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
platforms/group-charter-manager-api/src/controllers/GroupController.ts (1)
123-147: Unlock + delayed manual sync achieves the desired behavior but tightly couples to adapter internalsUsing
needsEVault = !group.charter && !group.ename && charterto gate eVault provisioning and then unlocking the group (via directlockedIds.splice) before callingupdateGroupensures that the first “real” charter+ename update can sync out. The follow‑upsetTimeoutthat re-fetches the full group and callsadapter.handleChangegives you a belt‑and‑suspenders sync so Cerberus sees complete data, but it may duplicate what the subscriber already does onafterUpdateand relies on direct mutation ofadapter.lockedIds.This is fine as a targeted workaround, especially with the explicit
// HACKcomment, though longer‑term you might want an explicitunlockhelper on the adapter and a dedicated “manual resync group(id)” path to avoid scattering adapter internals and timers across controllers.Also applies to: 149-155, 164-183
🧹 Nitpick comments (2)
platforms/group-charter-manager-api/src/web3adapter/watchers/subscriber.ts (1)
200-207: Guard for charterless groups looks correct; clarify semantics if empty charter should ever syncThe early return for
tableName === "groups" && !data.charterwill effectively prevent any group without a truthycharterfrom syncing, which matches the new “only sync once charter exists” flow. If you ever need to distinguish betweenundefined/nulland an intentionally-empty string charter, you may want to tighten this condition (e.g., check fordata.charter?.trim()), but as-is it seems consistent with how other code treats charters.platforms/cerberus/src/controllers/WebhookController.ts (1)
176-187: MakingprocessCharterChangefire‑and‑forget aligns with non‑blocking webhook handlingDropping
awaitand using.catchmeans charter-change processing no longer impacts the webhook’s HTTP status or latency, which directly addresses the “only triggers after edit” symptom by decoupling Cerberus from the main path. Just be aware this also means any failures here will be visible only via logs, not via webhook responses or retries; if reliability becomes a concern, you might later want a lightweight queue or metric around these async calls.Also applies to: 224-233
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
platforms/cerberus/src/controllers/WebhookController.ts(2 hunks)platforms/group-charter-manager-api/src/controllers/GroupController.ts(3 hunks)platforms/group-charter-manager-api/src/services/GroupService.ts(1 hunks)platforms/group-charter-manager-api/src/web3adapter/watchers/subscriber.ts(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
platforms/group-charter-manager-api/src/controllers/GroupController.ts (1)
platforms/group-charter-manager-api/src/web3adapter/watchers/subscriber.ts (1)
adapter(15-20)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: build
🔇 Additional comments (2)
platforms/group-charter-manager-api/src/services/GroupService.ts (1)
37-48: Charter-change detection and pre-save signature deletion are well-scopedThe
charterChangedcheck is precise (only fires when a new charter value is actually provided and differs) and deleting all signatures before the save ensures ORM subscribers and downstream processing never see stale signatures tied to the old charter. This nicely simplifies the flow to a single save while preserving correct invariants around signatures.platforms/group-charter-manager-api/src/controllers/GroupController.ts (1)
4-6: Group creation lock is a reasonable way to prevent premature syncs; verifyadapterexportLocking the group ID right after creation ensures subscriber‑driven syncs won’t push a charterless group out to the adapter, which fits the new provisioning flow. Just double‑check that
adapteris re‑exported from"../web3adapter"(and not only from the watcher module) so this import doesn’t create a brittle or circular dependency.Also applies to: 31-36
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
platforms/group-charter-manager-api/src/controllers/GroupController.ts (2)
149-155: Avoid mutatingadapter.lockedIdsdirectly; prefer an encapsulated unlock helperHere you’re reaching into
adapter.lockedIdsand manually splicing by index, while locking usesadapter.addToLockedIds(group.id). That asymmetry leaks adapter internals and risks breakage iflockedIdschanges shape or if IDs can be added more than once.Consider introducing a matching API (e.g.
adapter.removeFromLockedIds(id)or an internal helper) and using that here, or at least centralizing the filter/splice logic in one place rather than in the controller. Also worth double‑checking thatidfrom params is exactly the same type/format as the IDs stored viagroup.idto avoid unlock misses.
164-182: Manual delayed sync works but could use hardening and de‑couplingThe 5s
setTimeoutwith a re‑fetch andadapter.handleChangeis a pragmatic fix, but a few things are worth tightening:
- This may duplicate work with the normal watcher →
handleChangepath; ensureadapter.handleChangeis idempotent for group updates so double syncs are harmless.JSON.parse(JSON.stringify(completeGroup))assumes the entity is fully JSON‑safe (no Dates/BigInts/custom types); if the ORM provides a serializer (toJSON/toPlain), it would be safer to use that instead.- The 5s delay is a magic number; consider making it configurable if this hack sticks around.
Functionally this block looks fine and won’t block the HTTP response, but you might want to revisit these points before declaring the behavior “final”.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
platforms/group-charter-manager-api/src/controllers/GroupController.ts(3 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
platforms/group-charter-manager-api/src/controllers/GroupController.ts (1)
platforms/group-charter-manager-api/src/web3adapter/watchers/subscriber.ts (1)
adapter(15-20)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: build
🔇 Additional comments (2)
platforms/group-charter-manager-api/src/controllers/GroupController.ts (2)
5-5: Adapter import into controller looks appropriateImporting the shared
adapterhere keeps locking/unlocking and manual sync logic using the same singleton instance as the watcher setup, which is what you want for consistent Cerberus behavior.
31-33: Locking new groups on creation aligns with intended sync gatingLocking the group immediately after creation so it doesn’t sync until it has charter+ename fits the Cerberus fix (no premature “empty” group updates going out). The placement after
createGroupbut before participant mutations is reasonable.
Description of change
Cerberus (or rather group-charter-manager-api) had an issue that it only sent updates after you edited an already edited charter and there were no messages via cerberus before that.
Issue Number
Type of change
How the change has been tested
Change checklist
Summary by CodeRabbit
Performance
Bug Fixes
Improvements